[Refactor] Various Utilities, Assertion, and Concurrency Exception from server to libraries #6875
Conversation
nknize
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
owaiskazi19,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah and
xuezhou25
as code owners
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #6875 +/- ##
============================================
+ Coverage 70.52% 70.70% +0.17%
- Complexity 59112 59235 +123
============================================
Files 4814 4812 -2
Lines 283743 283717 -26
Branches 40915 40915
============================================
+ Hits 200110 200594 +484
+ Misses 67154 66615 -539
- Partials 16479 16508 +29
... and 495 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
nknize
added
v3.0.0
nknize
force-pushed
the
refactorUtilMethods1
branch
from
61f473a
to
5881606
Compare
Gradle Check (Jenkins) Run Completed with:
|
nknize
force-pushed
the
refactorUtilMethods1
branch
from
5881606
to
f674d09
Compare
nknize
force-pushed
the
refactorUtilMethods1
branch
from
f674d09
to
4e811c7
Compare
Gradle Check (Jenkins) Run Completed with:
|
|
oye.. bouncycastle FIPS WhiteSource failure bonanza continues... :/ |
Gradle Check (Jenkins) Run Completed with:
|
| @@ -79,6 +79,9 @@ dependencies { | |||
|
|
|||
| api "com.fasterxml.jackson.core:jackson-core:${versions.jackson}" | |||
|
|
|||
| // lucene | |||
| api "org.apache.lucene:lucene-core:${versions.lucene}" | |||
There was a problem hiding this comment.
@nknize I have doubts we need to depend on Lucene in any core / common modules, the BytesRefUtils is very specialized utility class, why do we need to promote it to core?
There was a problem hiding this comment.
Per #5910. Consider a plugin that has it's own custom Analyzer using AnalysisPlugin. Downstream plugins implement a TokenFilterFactory which needs Lucene's TokenStream. We need to take a dependency on lucene for this in whatever library houses those contracts (currently decided to be the opensearch-core library).
There was a problem hiding this comment.
Separately from that, I could remove BytesRefUtils altogether as that logic isn't actually used anywhere. I chose to keep it (for now) as a convenience function for external plugins.
There was a problem hiding this comment.
We need to take a dependency on lucene for this in whatever library houses those contracts (currently decided to be the opensearch-core library).
This is totally fine, and I believe by importing AnalysisPlugin the Lucene will be brought in, but the core does not need it and should not bring it I think. Take libs for example - they hardly ever need to know about Lucene but very likely will depend on core / common.
Separately from that, I could remove BytesRefUtils altogether as that logic isn't actually used anywhere.
👍 ty
There was a problem hiding this comment.
...but the core does not need it and should not bring it I think
I think we're not on the same page. #5910 plans to move general contracts like AnalysisPlugin (and all of the others) out of :server and into opensearch-core library so plugins only need take a dependency on the API not the entire :server module. To do that requires classes like *Plugin, be refactored out of :server and into libs:opensearch-core. If you refactor AnalysisPlugin to :opensearch-core you need :opensearch-core to take a dependency on lucene. So, the core library will need this lucene dependency, otherwise these contracts have to stay in :server
There was a problem hiding this comment.
What's the different route?
First thing, remove dependency to opensearch-core where opensearch-common would be sufficient (possibly moving few classes between modules). With that - we could be 1000% sure that no new transitive dependencies would surprise us along the away.
There was a problem hiding this comment.
In the meantime I updated this PR to remove opensearch-core dependency from ssl-config and nio libraries.
There was a problem hiding this comment.
@nknize libs/opensearch-cli ?
There was a problem hiding this comment.
I was hoping all of this would be done incrementally after this PR was merged.
There is an argument for moving the xcontent classes out of core into the common library now and letting core take the dependency on Lucene and other third-party libraries. Which would mean all of the plugins would revert the PRs they just merged. This is why we need to decouple downstream repos to enable us to move on core changes.
…om server to libraries To further reduce the surface area of utility methods in :server: this commit refactors the following: * MapBuilder from server module to common library * Assertions from server module to core library * BytesRef methods from CollectionUtils in server module to new BytesRefUtils in core library It also removes CollectionUtils dependency on hppc in prep for refactoring CollectionUtils methods to the proper library. Signed-off-by: Nicholas Walter Knize <[email protected]>
nknize
force-pushed
the
refactorUtilMethods1
branch
from
4e811c7
to
d89e8d4
Compare
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Nicholas Walter Knize <[email protected]>
Signed-off-by: Nicholas Walter Knize <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Nicholas Walter Knize <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-6875-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0a2009250cd857d68cdfa87340f8cd1b906d8c6e
# Push it to GitHub
git push --set-upstream origin backport/backport-6875-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.xThen, create a pull request where the |
…om server to libraries (opensearch-project#6875) To further reduce the surface area of utility methods in :server: this commit refactors the following: * MapBuilder from server module to common library * Assertions from server module to core library * BytesRef methods from CollectionUtils in server module to new BytesRefUtils in core library It also removes CollectionUtils dependency on hppc in prep for refactoring CollectionUtils methods to the proper library. Signed-off-by: Nicholas Walter Knize <[email protected]> Signed-off-by: Valentin Mitrofanov <[email protected]>
…om server to libraries (opensearch-project#6875) To further reduce the surface area of utility methods in :server: this commit refactors the following: * MapBuilder from server module to common library * Assertions from server module to core library * BytesRef methods from CollectionUtils in server module to new BytesRefUtils in core library It also removes CollectionUtils dependency on hppc in prep for refactoring CollectionUtils methods to the proper library. Signed-off-by: Nicholas Walter Knize <[email protected]> (cherry picked from commit 0a20092)
…om server to libraries (opensearch-project#6875) To further reduce the surface area of utility methods in :server: this commit refactors the following: * MapBuilder from server module to common library * Assertions from server module to core library * BytesRef methods from CollectionUtils in server module to new BytesRefUtils in core library It also removes CollectionUtils dependency on hppc in prep for refactoring CollectionUtils methods to the proper library. Signed-off-by: Nicholas Walter Knize <[email protected]> (cherry picked from commit 0a20092)
…om server to libraries (#6875) (#7612) To further reduce the surface area of utility methods in :server: this commit refactors the following: * MapBuilder from server module to common library * Assertions from server module to core library * BytesRef methods from CollectionUtils in server module to new BytesRefUtils in core library It also removes CollectionUtils dependency on hppc in prep for refactoring CollectionUtils methods to the proper library. Signed-off-by: Nicholas Walter Knize <[email protected]> (cherry picked from commit 0a20092) Signed-off-by: bansvaru <[email protected]> Co-authored-by: Nicholas Walter Knize <[email protected]>
To further reduce the surface area of utility methods in :server: this commit refactors the following:
It also removes CollectionUtils dependency on hppc in prep for refactoring CollectionUtils methods to the proper library.
related #5910